Solution#1480
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the mandatory React Router integration for syncing pagination with URL query parameters is missing, which blocks a core requirement of the task. In src/App.tsx, currentPage and perPage are initialized with hardcoded useState defaults instead of reading ?page and ?perPage from the URL on initial load, and there is no logic to update the URL when these values change. The same applies to src/components/Pagination/Pagination.tsx, where the component and its consumers do not use react-router-dom hooks (such as useSearchParams, or useLocation with useNavigate) to read or write page and perPage in the query string. To fix this, wrap the app in a BrowserRouter at the root, then in App.tsx derive initial currentPage/perPage from search params (with fallbacks of 1 and 5, and validation), and update the search params whenever page or per-page selection changes (e.g., always setting ?page=<currentPage>&perPage=<perPage>). Ensure that when perPage changes you reset to page 1 and immediately reflect page=1&perPage=<value> in the URL so a reload or bookmark restores the same view. Aside from the missing router behavior, your stateless Pagination component, active/disabled states, per-page selector, and info text are all implemented well, so once the URL syncing is in place the solution should fully meet the task requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| @@ -1,6 +1,17 @@ | |||
| { | |||
There was a problem hiding this comment.
The task requires using React Router to persist query params (?page=...&perPage=...) and apply them on page load. This file currently only imports React and useState. Import and use appropriate router hooks (for example, useSearchParams, useNavigate, or useLocation plus useEffect) so you can read params on mount and update them on changes. Also ensure the app is wrapped with a Router (e.g., BrowserRouter) at the root.
| "resolveJsonModule": true, | ||
| "isolatedModules": true, |
There was a problem hiding this comment.
Initial currentPage and perPage are hardcoded. According to the requirements, you must read page and perPage from the URL query params (if present) on load and use them to initialize state. Consider parsing search params in an effect or deriving initial values from useSearchParams.
| @@ -1,25 +1,45 @@ | |||
| import React from 'react'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
The task requires using React Router to persist and apply query params (e.g., ?page=2&perPage=7). There are no React Router imports or usage here. Import and use React Router hooks (for example, useSearchParams or useLocation/useNavigate) so the app can read query params on load and update them when page/perPage change.
| const [currentPage, setCurrentPage] = useState<number>(1); | ||
| const [perPage, setPerPage] = useState<number>(5); |
There was a problem hiding this comment.
currentPage and perPage are initialized from hardcoded useState values. Per requirements, these should be initialized from URL query parameters when present (fallback to defaults 1 and 5). Parse the query params on mount and set initial state accordingly.
| onChange={event => { | ||
| const newPerPage = Number(event.target.value); | ||
|
|
||
| setPerPage(newPerPage); | ||
| setCurrentPage(1); | ||
| }} |
There was a problem hiding this comment.
When the perPage selector changes you reset to page 1 and update state, which is correct, but you also must update the URL query params so ?page=1&perPage= is reflected in the address bar. Update the search params here (via router) in addition to updating state.
| <Pagination | ||
| total={total} | ||
| perPage={perPage} | ||
| currentPage={currentPage} | ||
| onPageChange={setCurrentPage} |
There was a problem hiding this comment.
Pagination's onPageChange is wired to setCurrentPage directly. You should wrap this so that when the page changes you both update component state and persist the new ?page value in the URL (keeping perPage in sync). This ensures reload/bookmark restores the same pagination state as required.
| @@ -1 +1,84 @@ | |||
| export const Pagination = () => {}; | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
The task requires using React Router to persist ?page=...&perPage=.... React Router hooks (e.g. useSearchParams or useLocation/useNavigate) are not imported here — you should import and use them to read/write query params.
| export const Pagination: React.FC<Props> = ({ | ||
| total, |
There was a problem hiding this comment.
Initial currentPage and perPage should be derived from the URL query parameters on page load (e.g. parse ?page and ?perPage), not always defaulted to 1 and 5. Add logic to initialize state from the router query params or set them after mount.
| > | ||
| « |
There was a problem hiding this comment.
When changing perPage you must also update the URL to reflect ?page=1&perPage=... so the selection is persisted. Currently this only updates component state (setPerPage / setCurrentPage(1)) but does not touch the router search params.
| onPageChange(page); | ||
| } | ||
| }} | ||
| > | ||
| {page} | ||
| </a> |
There was a problem hiding this comment.
When the page changes via the Pagination component you must sync that change into the URL query params (so ?page=...&perPage=... stays current). Either wrap setCurrentPage to also update the query params here or use an effect that synchronizes currentPage/perPage to the router.
2 participants
DEMO LINK